Fix jQuery missing issues for newer Sidekiq versions #181
Conversation
|
Sorry for the big delay and thanks for your contribution @hschne, I'll take a look and merge asap. |
|
|
||
| const updateCharts = () => { | ||
| if (noVisibleWorkers()) { | ||
| return | ||
| } | ||
|
|
||
| requestJSON(Charts.paths.realtime, { data: { excluded: Data.excludedWorkers } }) | ||
| .done(function (data) { | ||
| .then(response => response.json()) |
There was a problem hiding this comment.
It seems to me we don't exactly need this line here and also the 74. 🤔
There was a problem hiding this comment.
Hmm, could you go into more detail why not? I'm not able to follow.
updateCharts and initializeCharts are two distinct functions, both requesting distinct data. Both responses should arrive as JSON and thus would need to be converted, right? I feel like I may be missing something here 😅
There was a problem hiding this comment.
Nevermind @hschne, I supposed the response to be json already, sorry.
|
Closes #180 |
|
Hey @wenderjean , sorry for the delay on my part, I took a bit of a longer vacation 🏖️ I think everything should be in order now. If you are planning to properly move the other files off JQuery too I'd be happy to lend a hand 🙂 |
|
@hschne I created this other issue to track JQuery removal on |
First of all, let me say that I really love this gem, its super neat! 💯
I do have some trouble using it at the moment, however. When using
sidekiq-statisticwith Sidekiq 6.3 or later, it will be broken, as Sidekiq itself no longer ships with jQuery (see Changelog). This was also brought up in #180.Reproducing
Add
sidekiq6.3 or later andsidekiq-statisticto your gem file. Open the Sidekiq UI, head to the Statistic tab and observe errors in the console. Functionality such as graphs, polling or date picker will not work.Fix
First, because it was doable,
realtime_statistic.jswas moved off jQuery by replacing it with plain JS. If Sidekiq can do it, so can we! 😛Unfortunately, doing the same for
statistic.jswas not really feasible, as that would mean adding a new date picker and so on, and that is kind of a big change. Instead, I opted for loading Sidekiq from a CDN only if Sidekiq 6.3 or later is used.Let me know if you need any additional changes. I'd be happy to open any follow up PRs to also move
statistic.jsaway from jQuery, if you have any ideas there 🙂